-
Notifications
You must be signed in to change notification settings - Fork 0
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Luizmello/contacts app clean #2
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall, I think you understood the general architecture we had discussed - good job! You set everything up using those principles. I left comments around just so you could see the overall thoughts but I didn't get super granular because I think we don't need to at this point. There's more to learn and understand but we have a good basis to go off of now.
import SwiftUI | ||
|
||
struct CircleImage: View { | ||
var userImage: String |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should probably be named imageURL
.
Task(priority: .userInitiated) { | ||
do { | ||
let newPeople = try await self.fetchService.refreshPeople() | ||
self.dataModel.people = newPeople |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This could be combined with the line 38 like this:
self.dataModel.people = try await self.fetchService.refreshPeople()
final class Coordinator: ObservableObject { | ||
@Published var dataModel = DataModel() | ||
@Published var fetchService = FetchService() | ||
@Published var dataChanged: Bool = false |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This variable either isn't needed or should be renamed to something more usable, such as isFetching
because this would allow us to use this in other areas.
} | ||
} | ||
func refreshPeople() async { | ||
Task { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This probably shouldn't be in a Task
because it reduces the flexibility of this call. Rather we would use Task
at the call site.
struct UsersListView: View { | ||
@ObservedObject private var coordinator = Self.Coordinator() | ||
var body: some View { | ||
ZStack { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We don't need this.
await self.coordinator.refreshPeople() | ||
} | ||
} | ||
func move(from source: IndexSet, to destination: Int) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
func move(from source: IndexSet, to destination: Int) { | |
/// Move the person(s) from the given source index to the given destination location. | |
func move(from source: IndexSet, to destination: Int) { |
self.coordinator.dataChanged.toggle() | ||
} | ||
|
||
func removeRows(at offsets: IndexSet) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should probably be renamed to something more readable.
func removeRows(at offsets: IndexSet) { | |
func deletePersons(at offsets: IndexSet) { |
Spacer() | ||
} | ||
} | ||
struct contentBody: View { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No reason to do this. Because this isn't large, we can have this inside the detail view's body
or as a computed property in the details view..
var user: Person | ||
var body: some View { | ||
VStack(spacing: 5) { | ||
Text("\(user.name.first ?? "Luiz") \(user.name.last ?? "Mello")") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should probably use a computed property or lazy
property on the Person
object entitled displayName
to make this more readable.
func refreshPeople() async throws -> [Person] { | ||
return try await self.refreshPeopleFromAPI() | ||
} | ||
func refreshPeopleFromAPI() async throws -> [Person] { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be private to keep the underlying code inaccessible.
func refreshPeopleFromAPI() async throws -> [Person] { | |
private func refreshPeopleFromAPI() async throws -> [Person] { |
Summary
I created an application that makes an API request (using async/await) presenting a list of users and a user details navigation.
I tried to apply the Coordinator pattern shown earlier and I believe I was able to do it successfully. Still, there were some changes to be made that I couldn't make.
Items Completed